chore: Finish refactoring expression serde out of QueryPlanSerde
#2791
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2791 +/- ##
============================================
+ Coverage 56.12% 58.29% +2.17%
- Complexity 976 1411 +435
============================================
Files 119 162 +43
Lines 11743 14139 +2396
Branches 2251 2362 +111
============================================
+ Hits 6591 8243 +1652
- Misses 4012 4704 +692
- Partials 1140 1192 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| | `spark.comet.expression.ScalarSubquery.enabled` | Enable Comet acceleration for `ScalarSubquery` | true | | ||
| | `spark.comet.expression.Second.enabled` | Enable Comet acceleration for `Second` | true | | ||
| | `spark.comet.expression.Sha1.enabled` | Enable Comet acceleration for `Sha1` | true | | ||
| | `spark.comet.expression.Sha2.enabled` | Enable Comet acceleration for `Sha2` | true | |
There was a problem hiding this comment.
There is no new entry for UnscaledValue
| val optExpr = scalarFunctionExprToProtoWithReturnType( | ||
| "make_decimal", | ||
| DecimalType(expr.precision, expr.scale), | ||
| false, |
There was a problem hiding this comment.
| false, | |
| !expr.nullOnOverflow, |
There was a problem hiding this comment.
Thanks. I filed #2793 to investigate. For this PR, I wanted to just refactor and not make any functional changes.
There was a problem hiding this comment.
I see what changed now. Previously, we only supported MakeDecimal in the case that nullOnOverflow=true, but now we always support it. So this PR does introduce a functional change.
| // TODO PromotePrecision | ||
| // TODO KnownFloatingPointNormalized | ||
| // TODO ScalarSubquery | ||
| // TODO UnscaledValue |
There was a problem hiding this comment.
| // TODO UnscaledValue | |
| classOf[UnscaledValue] -> CometUnscaledValue |
| val value = expr.right | ||
| val bloomFilterExpr = exprToProtoInternal(bloomFilter, inputs, binding) | ||
| val valueExpr = exprToProtoInternal(value, inputs, binding) | ||
| if (bloomFilterExpr.isDefined && valueExpr.isDefined) { |
There was a problem hiding this comment.
nit: Pattern matching is a bit more idiomatic:
| if (bloomFilterExpr.isDefined && valueExpr.isDefined) { | |
| (bloomFilterExpr, valueExpr) match { | |
| case (Some(bf), Some(v)) => |
|
|
||
| import org.apache.comet.serde.QueryPlanSerde.{exprToProtoInternal, optExprWithInfo, scalarFunctionExprToProtoWithReturnType} | ||
|
|
||
| class CometUnscaledValue extends CometExpressionSerde[UnscaledValue] { |
There was a problem hiding this comment.
| class CometUnscaledValue extends CometExpressionSerde[UnscaledValue] { | |
| object CometUnscaledValue extends CometExpressionSerde[UnscaledValue] { |
andygrove
changed the title
QueryPlanSerde
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks @andygrove this is LGTM
| : : +- * BroadcastHashJoin Inner BuildRight (29) | ||
| : : :- * Filter (13) | ||
| : : : +- * HashAggregate (12) | ||
| : : : +- * CometColumnarToRow (11) |
There was a problem hiding this comment.
there is no fallback to Spark anymore?
There was a problem hiding this comment.
This PR added support for MakeDecimal in ANSI mode, although we still need to add tests.
| import org.apache.comet.serde.ExprOuterClass.Expr | ||
| import org.apache.comet.serde.QueryPlanSerde.{createBinaryExpr, exprToProtoInternal, optExprWithInfo, scalarFunctionExprToProto} | ||
|
|
||
| object CometReverse extends CometScalarFunction[Reverse]("reverse") { |
There was a problem hiding this comment.
Why is Reverse placed in strings.scala? In Spark, it belongs to collection_funcs.
https://github.com/apache/spark/blob/35a99f84a352ad995593d0a7a5a1433d235fcbc5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L1348-L1363
There was a problem hiding this comment.
Thanks. Moved to match Spark organization.
spark/src/main/scala/org/apache/comet/serde/CometScalarSubquery.scala
Outdated
Show resolved
Hide resolved
…y.scala Co-authored-by: Martin Grigorov <[email protected]>
Co-authored-by: Martin Grigorov <[email protected]>
Co-authored-by: Martin Grigorov <[email protected]>
Co-authored-by: Martin Grigorov <[email protected]>
…sion-comet into serde-refactor-few-more
5 participants
Which issue does this PR close?
Closes #2019
Closes #2670
Rationale for this change
Finish the expression serde refactor
What changes are included in this PR?
Mostly, just moving code to the serde framework.
The pattern matching for
MakeDecimalchanged slightly, and it no longer only matches onnullOnOverflow=true, so more plans run natively in Comet for Spark 4.0 now, fixing #2670There is a follow-on issue #2793
How are these changes tested?